fix(payment): approve the vault's real merkle charge, not the pool sum - #207
Merged
Conversation
A tester's rc.3 error (readable thanks to the un-truncation fix) showed payForMerkleTree reverting with 0xfb8f41b2 = ERC20InsufficientAllowance(address,uint256,uint256). Root cause: PaymentVaultV2.payForMerkleTree charges median(winner pool's 16 candidate amounts) * 2^depth, but ensureAllowance was fed maxPoolCost = the largest pool's SUM of candidates (~16 * median). For depth >= 5 the real charge exceeds that bound (2x at depth 5, 8x at depth 7, 64x at depth 10), so a wallet whose remaining allowance fell between the two skipped the approve and then reverted deterministically on every retry. Shallow trees and fat standing allowances masked the bug; drained allowances plus deep trees surface it. New merkleMaxCharge() mirrors the contract's math exactly - median at sorted index 8 (the contract's median16 k), times 2^depth, worst case across pools since the winner pool is selected on-chain and is unknowable client-side. (Upstream evmlib avoids the issue by approving U256::MAX; our bounded-allowance design just needs the correct bound.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A tester's merkle payment fails on every retry with (readable as of rc.3's un-truncation fix):
0xfb8f41b2decodes toERC20InsufficientAllowance(address,uint256,uint256)— the vault is authorized to pull less ANT than the payment needs, even thoughensureAllowancechecks (and tops up) before paying.Root cause
PaymentVaultV2.payForMerkleTreechargesmedian(winner pool's 16 candidate amounts) × 2^depth(median16= element 8 of the sorted 16). Our allowance check usedmaxPoolCost= the largest pool's sum of candidate amounts ≈ 16 × median.For depth ≥ 5 the real charge exceeds that bound — 2× at depth 5, 8× at depth 7 (a ~460MB / 119-chunk file), 64× at depth 10. Any wallet whose remaining allowance fell between the wrong bound and the real charge skipped the approve and then reverted deterministically. The bug hid behind two masks: shallow trees (charge ≤ pool sum for depth ≤ 4) and fresh standing allowances (1 ANT dwarfs typical charges); a drained allowance plus a deep tree exposes it.
Fix
New
merkleMaxCharge(depth, pools)mirrors the contract's math exactly: median at sorted index 8 (the contract'smedian16k), × 2^depth, worst case across pools — the winner pool is selected on-chain from sender+timestamp and is unknowable client-side, so the upper bound over pools is the tightest correct answer.ensureAllowancenow receives that instead of the pool sum.For context: upstream
evmlib's own wallet sidesteps this by approvingU256::MAX; our bounded-allowance design (standing 1 ANT, #191) just needs the bound to be correct.Testing
nuxi typecheckclean.🤖 Generated with Claude Code